Skip to content
This repository was archived by the owner on Mar 13, 2025. It is now read-only.

fix: issue #138 #141

Merged
merged 5 commits into from
Mar 18, 2021
Merged

fix: issue #138 #141

merged 5 commits into from
Mar 18, 2021

Conversation

yoution
Copy link
Contributor

@yoution yoution commented Mar 10, 2021

@maxceem please review

@yoution yoution mentioned this pull request Mar 10, 2021
Closed
@yoution
Copy link
Contributor Author

yoution commented Mar 16, 2021

@maxceem please review

@maxceem
Copy link
Contributor

maxceem commented Mar 17, 2021

sorry for the delay @yoution I will do it today

maxceem
maxceem suggested changes Mar 17, 2021
View reviewed changes
Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoution in general it works as expected, though there are a couple of visual fixes:

  1. Let's make all the disabled fields look the same way. Make all the fields look like "Job Name" when it's disabled.

    image

  2. When the Job Description is disabled because of "status" we should not show the red warning. We should only show this red warning when isApplicationPageActive === true.

    image

@yoution yoution requested a review from maxceem March 18, 2021 01:27
maxceem
maxceem suggested changes Mar 18, 2021
View reviewed changes
Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yoution the styles look great now.

I've missed one thing in the review before. When the skills select is disabled, can we hide the x buttons as we cannot remove these fields?

image

@yoution yoution requested a review from maxceem March 18, 2021 12:57
maxceem
maxceem approved these changes Mar 18, 2021
View reviewed changes
Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great now. Thank you @yoution.

@maxceem maxceem merged commit 249139a into topcoder-archive:dev Mar 18, 2021
@yoution yoution deleted the issue-138 branch June 22, 2021 09:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maxceem maxceem maxceem approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yoution @maxceem